Go runs this function before any test in the package

we serialize access to avoid problems while creating the project

this is to get the full function name of the test and extract the resource name

suggest improving both method names. This one sounds more like an init() and the other one I don't have a suggestion because I am not really sure I understand it. It looks like it's a getProjectID() kind of method

any suggestion? This method is the implementation of TestMain in the tests, it basically do some setup, execute the tests and teardown.

I called the methods TestMainExecution and ProjectIDExecution with the intention that Execution means during the test execution of the package. So for example ProjectIDExecution provides the id of a project that is created before the tests run and deleted after.

looking at what this file is managing, I think this should be about project resource initialisation.

TestMainExecution --> TestResourceInit
ProjectIDExecution --> GetOrCreateProjectID

I would avoid having this generic TestMain logic when in reality it is solely focusing on project resource re-usage (reminds me of YAGNI).

I still have not digested this file in full (as I mentioned somewhere, it looks too complicated), looping in the rest of the team @maastha @AgustinBettati @EspenAlbert @oarbusi

TestMainExecution is really for initialising and destroying resources, and calling test runs in the middle.

It's our custom implementation for TestMain adding resource lifecycle management

some info here about TestMain: https://medium.com/goingogo/why-use-testmain-for-testing-in-go-dafb52b406bc

I do agree that we should focus on more concrete naming so that it is more clear what is being done. I was more on the side of a name such as SetupSharedProject over TestMainExecution.

TestMainExecution is really for initialising and destroying resources

Regarding this point I was slightly confused because this function does not contain all the initialising logic, for example the creation of the project is handled in ProjectIDExecution. Was this for a specific reason?

yes, because i realized that for instance right now we always run all packages even if one test won't be run. so I lazy initialize the project creation in case test is being executed but doesn't need to get the execution project. in that case we don't need to create the project

How about ProjectIDForExecution or GetProjectIDForCurrentExecution or something?

Additionally, I think some methods/files could use more comments as to the "why/how", for example shared_resource.go (or the TestMain() but not sure if we need to do for every resource), as the mechanism could be hard for someone new to understand or use or extend completely or in case we run into any issues later.

I'm not sure about document in our code how Go features like TestMain work, but I'm fine to document more our code. I'm going to merge but please feel free to suggest doc, and i'll think in more docs in the following PRs, thx!

can't this class also be called by multiple places in parallel?

no, Go test calls TestMain and we call this function from there, it's like the main function but when running tests

ok noted. I am curious to know more: TestMain is called once per test package. So if in a subsequent PR you also add func TestMain(m *testing.M) {...} to another package, will this still work?
From how I am getting this (also based on your comment Go runs this function before any test in the package), it looks like there can be multiple TestMain calls from all the different test packages (which I think we have more than one)

yeah, the idea is to add TestMain to all packages.
when you run tests you choose the package, and as TestMain is a public function (uppercase) there can be only one TestMain for that package, and that's the function to be called when running the test for that package.

So it can't happen that two TestMain are run in a test execution, the execution is only for one package, and that can have at most one TestMain

reading this if makes me thinking that we're designing a non-trivial set of rules for which resources should be re-used or deleted. I see a potential risk here that tests could start randomly failing due to concurrency issues and figuring what happened is hard.

Let me start first with:

1. Why this if? What is the rationale?
2. Could we make things simpler? e.g. re-use resource for every test file and make it just a serial process?
3. Is there an alternative way to write this?

by default a project is created when you call ProjectIDExecution and it's deleted after all tests run. This if deletes the project created for this test execution.

i made a further optimization to allow reuse of a global project for that resource if defined so we avoid creating the project during the test run.

i'm not sure the code can be simplified to fulfil those requirements, but we'll probably learn more as we use this in other resources and might be able to simplify

i've simplified the if condition a little bit, let me know if it helps

1 - the if is to delete the project if it was created before running the tests
2 - we're reusing the resource (project) for all tests in the resource but we allow the tests to run in parallel as now so performance is not affected
3 - surely it can be written in other ways, but I think this approach is ok at the moment, we'll probably learn more soon and will be able to improve it. any suggestions?

this flags that we created a project for the test execution and we'll need to delete it after all tests finish

I am probably on the side of making things as simple as possible. Can't we just pass the name of the resource when things are initialised?

i think this is less error prone than passing the resource name as a string in each call. i think it's better if it can be got automatically.

(something similar happens in the GH Actions where we have some errors from time to time because copying/pasting incorrectly to pass redundant info)

ok noted, this method makes me thinking that even if this is code written for testing infra I think it should be unit tested. If we don't test it, I suspect it will become hard to touch for other engineers.

also this method is not really very important, it could fail and return "" and everything would work (except trying to reuse the global project for that resource).

but you have a point, let me see if it can be tested, it's a bit tricky because it depends on the resource test structure.

[nit] this looks like a more generic util method to be put outside this file.

moved

[q] Why are we removing the logic of having a Global project? I would imagine we would have a mix of both have a global project, and also projects that are created per resource execution

because I realized that projects can't have more than 25 clusters so I don't think that's an option any more, at least for generic projects that don't need any special configuration.
for projects than need some setup (like triggers) we'll probably have a global project for triggers.

but in the resource in this PR it doesn't need any special config so i think it's better not to use a global project for it (it can use a global project for that resource, but not shared by all/many resources)

[q] I am struggling to understand in which case resourceName would be empty. If it is empty it is likely an indicator that TestMainExecution was not called, and in this case I would opt for stopping the execution all together.

it's just a precaution, imagine a resource doesn't follow the expected structure. but i agree it shouldn't happen but in some edge cases

Is there a possibility of having a separate function for providing the global project? I believe this would help simplify this function and make its usage more clear. In that case I understand that needsDeletion property could be removed as well.

i had in mind that projects just ask for a project and a new one or a resource-global one is given but they don't need to know. and there is a nice fallback mechanism so you can create global-resource clusters for the most demanding resources and maybe for resources that just create a couple of projects you don't want to create a global-resource one as the benefit is not so big and you need that project always created.

do you think it's better to remove this abstraction and projects decide if they want an execution project or a global-project?

in any way there will be a way to ask for specific projects when they need some extra config like triggers, but i don't think it's so useful to allow this for regular projects

don't handle the exist status internally let TestMain handle that which is supposed to do

it's a bit more verbose but I like the idea and it's more explicit, will change to something like:

func TestMain(m *testing.M) {
	acc.SetupSharedResources()
	exitCode := m.Run()
	acc.CleanupSharedResources()
	os.Exit(exitCode)
}

thx!

all functions like this will start with ProjectID, we have ProjectIDExecution, ProjectIDGlobal and might have ProjectIDTrigger, etc.

can you make sure all assertions where you do string formatting end with f for format, other wise is a concatenation

I think they do the same, e.g.:

a := "hello"
	require.Empty(t, a, "a is not empty: %s, watchout", a)

Error:      	Should be empty, but was hello
        	Test:       	TestAccNetworkPrivatelinkEndpointServiceDataFederationOnlineArchive_basic
        	Messages:   	a is not empty: hello, watchout

the signature is: func Empty(t TestingT, object interface{}, msgAndArgs ...interface{}) {

example of implementation:

//	assert.NotNilf(t, err, "error message %s", "formatted")
func NotNilf(t TestingT, object interface{}, msg string, args ...interface{}) bool {
	if h, ok := t.(tHelper); ok {
		h.Helper()
	}
	return NotNil(t, object, append([]interface{}{msg}, args...)...)
}

i think it's just to make sure that the first param is a string (with the msg)

yeah I see internally does fmt.Sprintf(msgAndArgs[0].(string), msgAndArgs[1:]...) I assumed this was closer to t.Error vs t.Errorf 🤷

out of curiosity why create is a helper but delete is not?

the creation is done lazily inside a test so we have the T, but deletion is done in TestMain when all test have finished and we don't have test context anymore

which is delete is a helper you can then do

can't be because it's called from TestMain not from a test

I see, this may make this approach a bit noisy on the logs since you usually only want logs when running on verbose mode (-v) and right now with printf is harder to control

we're running with -v in GH Actions, but you have a point, will consider in next PRs if delete it, thanks!

how are we running tests? normally you want logs when running in verbose mode -v

we use verbose flag:

TF_ACC=1 go test $(ACCTEST_PACKAGES) -run '$(ACCTEST_REGEX_RUN)' -skip '$(ACCTEST_REGEX_SKIP)' -v -parallel $(PARALLEL_GO_TEST) $(TESTARGS) -timeout $(ACCTEST_TIMEOUT) -ldflags="$(LINKER_FLAGS)"

yeah I see internally does fmt.Sprintf(msgAndArgs[0].(string), msgAndArgs[1:]...) I assumed this was closer to t.Error vs t.Errorf 🤷

I see, this may make this approach a bit noisy on the logs since you usually only want logs when running on verbose mode (-v) and right now with printf is harder to control

Maybe support os.Getenv("MONGODB_ATLAS_PROJECT_ID") similar to acc.GetClusterInfo?

I think that is a good idea, but i'm going to do in a next PR so this doesn't get more complicated, thx!